Skip to content

fix: def declaration within function#9379

Merged
manzt merged 27 commits into
marimo-team:mainfrom
Shamik-07:fix/def_declaration
May 19, 2026
Merged

fix: def declaration within function#9379
manzt merged 27 commits into
marimo-team:mainfrom
Shamik-07:fix/def_declaration

Conversation

@Shamik-07

@Shamik-07 Shamik-07 commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

📝 Summary

Previously, F12/command usage would jump effectively using first match semantics and could jump to an outer/global definition when the cursor was on a local symbol use. Now, the jump follows lexical scope first, and fixes the issue where symbols inside def blocks did not resolve correctly.

Tested with the code snippets to reproduce this issue and added the same to the tests.
Closes #1430

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Tests have been added for the changes made.

I have read the CLA Document and I hereby sign the CLA

@vercel

vercel Bot commented Apr 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment May 15, 2026 8:12pm

Request Review

@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@Shamik-07

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Shamik-07

Copy link
Copy Markdown
Contributor Author

recheck

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="frontend/src/core/codemirror/go-to-definition/utils.ts">

<violation number="1" location="frontend/src/core/codemirror/go-to-definition/utils.ts:81">
P1: The new local-first short-circuit treats any local symbol occurrence as a successful local definition, which can block cross-cell definition jumps.</violation>
</file>

<file name="frontend/src/core/codemirror/go-to-definition/commands.ts">

<violation number="1" location="frontend/src/core/codemirror/go-to-definition/commands.ts:87">
P2: Class scope is incorrectly treated as an enclosing scope for method symbol resolution, which can send go-to-definition to class-body names instead of module/global definitions.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant U as User
    participant V as Editor View (CodeMirror)
    participant UT as utils.ts
    participant C as commands.ts
    participant T as Syntax Tree (Lezer)
    participant S as Notebook Store (Jotai)

    U->>V: Trigger "Go to Definition" (F12/Cmd+Click)
    V->>UT: goToDefinitionAtCursorPosition(view)
    
    UT->>UT: getWordUnderCursor(state)
    Note right of UT: CHANGED: Now returns both<br/>variable name AND position

    UT->>C: NEW: goToVariableDefinition(view, name, position)
    
    C->>T: resolveInner(position)
    T-->>C: SyntaxNode
    
    C->>C: NEW: getScopeChain(tree, position)
    Note right of C: Build hierarchy of scopes<br/>(Function, Class, Global)

    C->>C: NEW: collectMatchingDeclarations(tree)
    C->>T: iterate tree nodes
    T-->>C: VariableName, ParamList, etc.

    C->>C: NEW: findScopedDefinitionPosition()
    Note right of C: Filter declarations by lexical<br/>scope and position sensitivity

    alt Local Definition Found
        C->>V: focus() and setSelection(foundPosition)
        C-->>UT: true
    else Not Found Locally (Reactive/Global Fallback)
        C-->>UT: false
        UT->>S: getEditorForVariable(variableName)
        Note over UT,S: Checks marimo variablesAtom<br/>for definitions in other cells
        
        alt Found in Other Cell
            S-->>UT: Target Editor View
            UT->>C: findFirstMatchingVariable(targetView, name)
            C->>T: iterate tree (first match)
            T-->>C: position
            C->>V: focus target view and setSelection
            UT-->>V: true
        else Not Found Anywhere
            S-->>UT: null
            UT-->>V: false
        end
    end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread frontend/src/core/codemirror/go-to-definition/utils.ts
Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes CodeMirror “go to definition” so symbol resolution prefers lexical scope (e.g., inside def blocks) before falling back to reactive/global definitions across cells, and adds tests for the reported regressions.

Changes:

  • Enhance go-to-definition to pass usage position and attempt local (scoped) resolution first.
  • Implement scoped definition lookup in goToVariableDefinition (functions, lambdas, comprehensions, etc.).
  • Add frontend tests covering local-vs-global resolution and private-variable behavior; broaden Altair chart config helper input types.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
marimo/_data/charts.py Expands add_common_config to accept FacetChart.
frontend/src/core/codemirror/go-to-definition/utils.ts Threads usage position from cursor selection into go-to-definition flow.
frontend/src/core/codemirror/go-to-definition/commands.ts Adds scoped AST-based declaration collection and resolution logic.
frontend/src/core/codemirror/go-to-definition/tests/utils.test.ts Adds tests ensuring local definitions are preferred over reactive globals and private vars stay in-cell.
frontend/src/core/codemirror/go-to-definition/tests/commands.test.ts Adds tests for selecting in-scope local and parameter definitions.

Comment thread frontend/src/core/codemirror/go-to-definition/utils.ts
Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts
Comment thread marimo/_data/charts.py
def add_common_config(chart: alt.Chart | alt.LayerChart) -> alt.Chart:
def add_common_config(
chart: alt.Chart | alt.LayerChart | alt.FacetChart,
) -> alt.Chart:
@mscolnick mscolnick requested a review from manzt April 27, 2026 13:24
@Shamik-07

Copy link
Copy Markdown
Contributor Author

@manzt
Do these copilot reviews need to be addressed ?

Three known bugs in the new scope-aware go-to-definition path.

Set comprehensions never create a scope because SCOPE_CREATING_NODES
uses "SetComprehension" but the Lezer Python grammar emits
"SetComprehensionExpression" — go-to-def on the expression `x` in
`{x for x in ...}` falls back to first-match and lands on an outer
`x` instead of the for-target. A dict-comprehension test uses the
grammar-correct name and passes as a positive control.

`getScopeChain` walks straight up the syntax tree and pushes any
ClassDefinition ancestor onto the chain, but in Python a method
body does not see its enclosing class scope. Once a function or
lambda boundary has been crossed walking outward, class scopes must
be skipped. With the current behavior, go-to-def on `x` inside a
method finds the class-body `x` instead of the module-level `x`.

POSITION_SENSITIVE_SCOPES includes "global", so a module-level
declaration whose position is after the usage is filtered out. From
inside a function that forward-references a later top-level name,
the lookup returns null and the fallback first-match lands on the
usage itself. Class-scope semantics need the position filter; module
scope does not.

The utils.test.ts mocks also drop the `as never` casts in favor of
fully-populated CellHandle literals, so a future field addition will
surface as a typecheck error.

@manzt manzt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Left a few inline comments on edge cases I think aren't covered yet. Pushed failing tests for them at fix/def_declaration-tests.

Comment thread frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts Outdated

case "ArrayComprehensionExpression":
case "DictionaryComprehensionExpression":
case "SetComprehension":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, I believe this is SetComprehensionExpression (like the others) and will never get hit. A test with a set comprehension should have caught this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts Outdated
Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts Outdated
Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts
Add failing tests for set-comprehension and forward-ref scope bugs
@Shamik-07

Copy link
Copy Markdown
Contributor Author

Looks good. Left a few inline comments on edge cases I think aren't covered yet. Pushed failing tests for them at fix/def_declaration-tests.

Thank you very much for helping out.
I have addressed your comments. Please let me know if there's something else.

Screen.Recording.2026-05-11.at.19.03.24.mov

@Shamik-07 Shamik-07 requested a review from manzt May 11, 2026 23:06
@mscolnick mscolnick changed the title Fix/def declaration within function fix: def declaration within function May 15, 2026
@manzt manzt added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels May 19, 2026

@manzt manzt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution!

@manzt manzt merged commit 26604de into marimo-team:main May 19, 2026
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go to declaration does not work within def

3 participants